BA Plan & log transaction #54
Conversation
nirbar
force-pushed
the
nirbar-5386-plan-log
branch
from
652f7fa to
d3c3fd1
Compare
| { | ||
| DWORD cbSize; | ||
| LPCWSTR wzTransactionId; | ||
| BOOL fTransaction; |
There was a problem hiding this comment.
| MSIHANDLE hMsiTrns = NULL; | ||
| HANDLE hMsiTrnsEvent = NULL; |
There was a problem hiding this comment.
Why are you insisting on these changes?
| LogId(REPORT_STANDARD, MSG_MSI_TRANSACTION_BEGIN, pRollbackBoundary->sczId); | ||
|
|
There was a problem hiding this comment.
Why are you insisting on these changes?
| PackageFindRollbackBoundaryById(pPackages, sczId, &pRollbackBoundary); | ||
| ExitOnFailure(hr, "Failed to find rollback boundary: %ls", sczId); | ||
|
|
||
| pRollbackBoundary->sczLogPath = sczLogPath; | ||
|
|
||
| hr = MsiEngineBeginTransaction(pRollbackBoundary); | ||
| hr = MsiEngineBeginTransaction(szName, &hMsiTrns, &hMsiTrnsEvent, sczLogPath); | ||
| ExitOnFailure(hr, "Failed beginning an MSI transaction"); |
There was a problem hiding this comment.
Why are you insisting on these changes?
| __in BURN_ROLLBACK_BOUNDARY* pRollbackBoundary | ||
| __in LPCWSTR wzName, | ||
| __out MSIHANDLE *phTransactionHandle, | ||
| __out HANDLE *phChangeOfOwnerEvent, | ||
| __in_z LPCWSTR szLogPath |
There was a problem hiding this comment.
Why are you insisting on these changes?
There was a problem hiding this comment.
My comment here was partially about functionality and partially about aesthetics: #29 (comment). Unfortunately, it seems like I didn't do a good job about helping you understand that I made the aesthetical changes myself. So you ended up spending a lot of time fighting merge conflicts to bring your changes back here. The end result is you are reverting the aesthetical changes I made while not implementing any more functionality.
I guess it's up to you whether to spend the time to undo them, but these are the kind of changes that make it so I have to manually merge your PR. Since manually merging takes a lot more time, it would have to wait until I'm interested in doing that. I hope you understand that, and I hope you will consider contributing your other v3 changes even if this PR doesn't get merged any time soon.
| BOOL fTransaction; | ||
| BOOL fActiveTransaction; // only valid during Apply. | ||
| LPWSTR szLogPathVariable; |
There was a problem hiding this comment.
Should be sczLogPathVariable.
| hr = WiuInitialize(); | ||
| NativeAssert::Succeeded(hr, "Failed to initliaze wiutil."); | ||
|
|
There was a problem hiding this comment.
Why are you insisting on these changes? #29 (comment)
|
I'm not insisting on anything. Rather, I got lost with all the changes you're insisting on and simply pushed what I had. |
|
I'm sorry if I'm coming off as being hard to work with. I do appreciate what you're doing and I want your changes. What I'm having trouble with is striking a balance between giving review comments for you to fix versus me fixing everything for you. I spent a significant amount of time a few months ago fixing your original v4 pull request from 2016. Fixing the comments that I put on that original PR. Then I spent a significant amount of time taking your previous changes. If you don't want to spend any more time on these changes, then that's fine. They're additive so I can get to them when I have time. What I'm really interested in are your other v3 changes. Especially the breaking ones, like only allowing MsiPackage and MspPackage inside of MSI transactions. |
|
I don't mind doing the PR if you don't mind focusing on functionality |
nirbar
force-pushed
the
nirbar-5386-plan-log
branch
from
d3c3fd1 to
72c3c75
Compare
|
I incorporated all of your changes into wixtoolset/wix#34. Hopefully it will be easier for you to contribute your other changes now that we're back to a single repo. Thanks. |
No description provided.